Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#321 search page bug #322

Merged
merged 7 commits into from
Nov 29, 2023
Merged

#321 search page bug #322

merged 7 commits into from
Nov 29, 2023

Conversation

BelousSofiya
Copy link
Collaborator

@BelousSofiya BelousSofiya commented Nov 23, 2023

I found some problems on our site.

  1. On search page even your own company had a "like"-star. It doesn't work - you can't add your company to the saved-list. But you see this star. I've fixed it
  2. On search page didn't work links to pages with detailed information about company. I've fixed it
  3. link /admin redirected us to the inbuild django admin, so our custom admin was not available. I've changed it and for now you can see our custom admin by link /customadmin and Django admin by link /admin
    I rebuild our site from this branch, so you can see my changes "in nature"

);
)
.then((res) => res.data);
setUserId(userData.id);
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use existing useUser hook instead of adding userData here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think, we need additional request to api/users/me?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useUser hook provides all the information about authorized user. And you can access this data when you need it.
It is common practice to use this hook. And it's no need to duplicate the logic of getting user id.
And we get that information via request to api/users/me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I as authorized user put smth to the searchpage and press enter iuseProvideAuth sends the request to api/users/me. So I use it. If I'll use useUser it will send this request again.

Copy link
Collaborator

@Lvyshnevska Lvyshnevska Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have duplicated logic in our code)
I'd recommend to wait for Oleg review

Copy link
Collaborator Author

@BelousSofiya BelousSofiya Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change code, but I'm not sure in this solution. We are waiting for Oleg))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have useUser hook. My proposition is to use it instead of adding userData in useProvideAuth.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Lvyshnevska , inside a useUser hook we utilize a useSWR which should cache fetched data and prevent second API call. There is no need to call /api/auth/users/me/ the second time if it was already called and cached in the useUser

Copy link
Collaborator Author

@BelousSofiya BelousSofiya Nov 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Done. But both hooks useUser and useProvideAuth use the same request /api/auth/users/me/ to the backend. Is it ok? I mean if I can use only useUser to check authenticated user or not and to get his data (id for my case), why do we need useProvideAuth?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as what I see we return different values from these hooks. But since they use the same API I assume that we can combine them both into one hook. Also, we do not use SWR in useProvideAuth that's something that could be fixed

@BelousSofiya BelousSofiya requested review from popovycholeg and removed request for popovycholeg November 24, 2023 16:56
@BelousSofiya BelousSofiya merged commit 0d656b0 into develop Nov 29, 2023
6 checks passed
@popovycholeg
Copy link
Collaborator

Approved, but would be happy to resolve tech debt between useProvideAuth and useUser. Could you create a ticket for it? @BelousSofiya

@BelousSofiya BelousSofiya deleted the #321SearchPageBug branch November 29, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants